Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: correct the formula for interpolating between bin start and end (interpolatedSignalRef) #9111

Merged
merged 6 commits into from
Sep 29, 2023

Conversation

kanitw
Copy link
Member

@kanitw kanitw commented Sep 29, 2023

The new formula correctly makes bandPosition = 0 represent start and bandPosition = 1 represents end.
(The existing code wasn't consistent with the simplified formula when bandPosition = 0 or 1)

if (bandPosition === 0 || bandPosition === 1) {
    ref.scale = scaleName;
    const field = bandPosition === 0 ? start : end;
    ref.field = field;
  }

The new formula correctly makes bandPosition = 0 represent start and bandPosition = 1 represents end.

(The existing code wasn't consistent with the simplified formula when bandPosition = 0 or 1

```
if (bandPosition === 0 || bandPosition === 1) {
    ref.scale = scaleName;
    const field = bandPosition === 0 ? start : end;
    ref.field = field;
  }
```
@kanitw kanitw requested review from yhoonkim, domoritz, a team and lsh September 29, 2023 03:24
@lsh
Copy link
Member

lsh commented Sep 29, 2023

Code looks good but I'd encourage checking to make sure this doesn't result in any stale documentation. Alternatively if this is now an invariant, maybe we should be documenting it?

@kanitw kanitw changed the title fix: correct formula for interpolatedSignalRef fix: correct formula for interpolating between bin start and end (interpolatedSignalRef) Sep 29, 2023
@kanitw
Copy link
Member Author

kanitw commented Sep 29, 2023

Btw, after checking more, I can confirm that the resulting diff is correct.

(The formula was indeed wrong. One example is that in the old code, band=0.99 is wider than band=1, simply because the offset to add 1 pixel is applied in the wrong direction due to the wrong formula.

Before

image image

After

image

Thus, the diff that makes the bar narrower is similarly correct

image

Copy link
Member

@lsh lsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked through the diff and no complaints.

@kanitw kanitw changed the title fix: correct formula for interpolating between bin start and end (interpolatedSignalRef) fix: correct the formula for interpolating between bin start and end (interpolatedSignalRef) Sep 29, 2023
@kanitw kanitw merged commit d4b27bb into main Sep 29, 2023
8 of 10 checks passed
@kanitw kanitw deleted the kw/fix-interpolatedSignalRef branch September 29, 2023 23:26
alliefeldman pushed a commit to alliefeldman/vega-lite that referenced this pull request Oct 10, 2023
…(interpolatedSignalRef) (vega#9111)

Co-authored-by: GitHub Actions Bot <[email protected]>
BradyJ27 pushed a commit to BradyJ27/vega-lite that referenced this pull request Oct 19, 2023
…(interpolatedSignalRef) (vega#9111)

Co-authored-by: GitHub Actions Bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants